Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-43572: [JS] Move most dependencies to the devDependencies #44517

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sgonyea
Copy link

@sgonyea sgonyea commented Oct 23, 2024

Rationale for this change

Adding apache-arrow as a dependency causes its dependencies to interact with the projects own dependencies. It's also bloating up JS build sizes.

Moving these dependencies to devDependencies will resolve this issue.

What changes are included in this PR?

Moves most dependencies to devDependencies in the package.json.

Are these changes tested?

n/a

Are there any user-facing changes?

This does not change user-facing APIs.

Copy link

⚠️ GitHub issue #43572 has been automatically assigned in GitHub to PR creator.

@domoritz
Copy link
Member

This probably breaks the cli, no? I also think it's common to have typescript typings as dependencies.

Iirc there was a similar pr recently that didn't work.

@sgonyea
Copy link
Author

sgonyea commented Oct 24, 2024

@domoritz Thanks for the review! I updated the PR to restore some of the imports used in the bin commands.

It looks like the only js/bin being exported is the arrow2csv. But I restored the deps being imported directly in the js/bin to be safe.

@domoritz
Copy link
Member

I think we need tslib and the helpers, though, no?

@domoritz
Copy link
Member

Here is the pull request I was tinking of with discussion of why we need the dependencies: #43215. We could make the package lighter by removing the CLI tool from the public API. Would you like to make a pull request for that?

Comment on lines +68 to +69
"@types/command-line-args": "^5.2.3",
"@types/command-line-usage": "^5.0.4",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are indeed not needed since the cli doesn't expose types I think

Comment on lines +65 to +67
"@types/node": "^20.13.0",
"tslib": "^2.6.2",
"@swc/helpers": "^0.5.11",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are dependencies since they are needed at runtime or for correct types.

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Oct 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants